Skip to content

Openshift - datetime value handling #889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
timabbott merged 4 commits into from
Dec 5, 2025
Merged

Openshift - datetime value handling #889

timabbott merged 4 commits into from
Dec 5, 2025

Conversation

@Niloth-p
Copy link
Contributor

@Niloth-p Niloth-p commented Nov 30, 2025

We want to use only ISO 8601 formats with the <time:> syntax, so we remove the previous comment recommending the usage of <time:dep_time>, and instead add a (commented-out) function that converts the datetime to the right format.

How did you test this PR?
I did not.

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

timabbott
timabbott reviewed Dec 2, 2025
View reviewed changes
@Niloth-p Niloth-p force-pushed the openshift-datetime-field branch from 661050c to 146fadd Compare December 4, 2025 19:45
@zulipbot zulipbot added size: M and removed size: S labels Dec 4, 2025
@Niloth-p
Copy link
Contributor Author

Niloth-p commented Dec 4, 2025

Updates:

  • Added a datetime_to_global_time helper in zulip/zulip/__init__.py.
  • Uncommented the function, and edited the comment above it.

This version still uses strptime(). Since OpenShift's datetime strings use a space instead of the "T".
I have another branch with a version that uses fromisoformat() (the backported one) here just in case.

@Niloth-p Niloth-p force-pushed the openshift-datetime-field branch 3 times, most recently from d06270a to 8b65699 Compare December 4, 2025 23:16
andersk
andersk reviewed Dec 4, 2025
View reviewed changes
andersk
andersk reviewed Dec 5, 2025
View reviewed changes
@Niloth-p
openshift: Allow using the deployment ID and time in the message.
261fc93 
While the message template customization function had `dep_time` and
`dep_id` parameters, the `deployment` dict was not populated with those
fields previously.
@Niloth-p
Add a datetime_to_global_time helper function.
8a165f1
@Niloth-p Niloth-p force-pushed the openshift-datetime-field branch from 8b65699 to 572de66 Compare December 5, 2025 01:01
@Niloth-p Niloth-p force-pushed the openshift-datetime-field branch from 572de66 to 59fa607 Compare December 5, 2025 01:09
@Niloth-p
Copy link
Contributor Author

Niloth-p commented Dec 5, 2025

Updates:

  • Removed the string replacement
  • Removed __all__

Apologies for the embarrasing errors. I even checked that docs page earlier. 🤷
Thank you for the patient review, and the help with the re-export, @andersk.

Tangential questions (if you have the time):

  • Ruff's doc on PLC0414 says that rule does not apply in __init__.py files. But, it still raised that error, not sure if I'm misunderstanding it.
  • Weirdly, the previous GitHub CI runs that ended with mypy errors did not raise any issues when locally running .tools/lint. What could be causing that mismatch? Shouldn't the activated venv be enough to make the results uniform?

@timabbott timabbott merged commit 1ca4eff into zulip:main Dec 5, 2025
14 checks passed
@timabbott
Copy link
Member

timabbott commented Dec 5, 2025

Looks good to me, merged, thanks @Niloth-p!

I encourage discussing those tangential questions in new #integrations threads in chat.zulip.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andersk andersk andersk left review comments

@timabbott timabbott timabbott left review comments

Assignees

No one assigned

Labels

size: M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Niloth-p @timabbott @andersk @zulipbot